sdk: tighten ReadableSpan.attributes to non-Optional Mapping (#4569)#5183
sdk: tighten ReadableSpan.attributes to non-Optional Mapping (#4569)#5183WatchTree-19 wants to merge 1 commit into
Conversation
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Thanks @WatchTree-19 - left some feedback on things we need to tidy up
|
|
||
| ## Unreleased | ||
|
|
||
| - `opentelemetry-sdk`: tighten `ReadableSpan.attributes` annotation to non-Optional `Mapping` so callers don't need `assert ... is not None` boilerplate; runtime guarantee was already in place via `MappingProxyType(self._attributes or {})` |
There was a problem hiding this comment.
Looks like you got some other changelog entries in here - please can you clean this up?
| @property | ||
| def attributes(self) -> types.Attributes: | ||
| def attributes(self) -> Mapping[str, types.AttributeValue]: | ||
| # The implementation always returns a MappingProxyType, never None, |
There was a problem hiding this comment.
This is very verbose, can we tighten this up?
|
|
||
| @property | ||
| def attributes(self) -> types.Attributes: | ||
| def attributes(self) -> Mapping[str, types.AttributeValue]: |
There was a problem hiding this comment.
Do / should we update the API definition too?
github-project-automation
Bot
moved this to Reviewed PRs that need fixes
in Python PR digest
…try#5183) Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com>
|
thanks @MikeGoldsmith. pushed a follow-up addressing all three:
@property
def attributes(self) -> Mapping[str, types.AttributeValue]:
# `or {}` keeps the return non-None; see #4569.
return MappingProxyType(self._attributes or {})
let me know if there's anything else. |
|
Thanks for the PR! Just a heads-up: we no longer update Please add the appropriate changelog fragment for this change instead of editing |
|
@emdneto already addressed in the follow-up - reverted the |
…n-telemetry#4569) The implementation always returns a `MappingProxyType`, never `None`, because `self._attributes or {}` falls back to an empty dict. Tightening the return annotation lets callers index `span.attributes["key"]` without type-checker complaints. Adds the towncrier fragment per CONTRIBUTING. Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com>
WatchTree-19
force-pushed
the
fix/readable-span-attributes-non-optional
branch
from
6b96015 to
6e05d7d
Compare
|
@MikeGoldsmith @emdneto gentle bump - rebased onto current main to clear the |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
3 participants
Description
ReadableSpan.attributesis annotated astypes.Attributes, which resolves toOptional[Mapping[str, AttributeValue]]. The implementation always returnsMappingProxyType(self._attributes or {})- theor {}fallback ensures we never return None at runtime.Pyright / Pylance flags
Object of type "None" is not subscriptableonspan.attributes["key"], forcingassert span.attributes is not Noneboilerplate at every call site.This PR tightens the return annotation on
ReadableSpan.attributesto the non-OptionalMapping[str, types.AttributeValue]. Implementation unchanged.Mappingis already imported in the file.Scope:
ReadableSpan.attributesonly. Inherited bySpanand_Span, no further changes needed.Event.attributesandLink.attributesreturnself._attributesdirectly (which can be None) and stay Optional.types.Attributesglobal alias is unchanged for the same reason.Per the comment thread on #4569 from @exekis and @tekumara, the implementation has never returned None from
ReadableSpan.attributes, so this is type-system tightening rather than a behaviour change. Technically a breaking change for any caller explicitly handling None on the return; in practice there shouldn't be any.Fixes #4569
Type of change
How Has This Been Tested?
python -m astDoes This PR Require a Contrib Repo Change?
Checklist: